Fix: limit stored request events to 1000 per app#2025
Fix: limit stored request events to 1000 per app#2025kanishka0411 wants to merge 2 commits intogetAlby:masterfrom
Conversation
📝 WalkthroughWalkthroughAdds pruning of stored request events in the Nip47 event handler: introduces Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant Handler as HandleEvent
participant Pruner as pruneRequestEvents
participant DB as Database
App->>Handler: Send request/event
Handler->>DB: Update app.last_used_at
Handler->>Pruner: Spawn async pruneRequestEvents(app.ID)
Note over Pruner,DB: Runs in goroutine
Pruner->>DB: Query request_events for app (ordered by id asc)
DB-->>Pruner: Return rows beyond MaxRequestEventsPerApp
Pruner->>DB: DELETE old request_events (LIMIT ...)
DB-->>Pruner: Deletion result
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 🧪 Unit Test Generation v2 is now available!We have significantly improved our unit test generation capabilities. To enable: Add this to your reviews:
finishing_touches:
unit_tests:
enabled: trueTry it out by using the Have feedback? Share your thoughts on our Discord thread! Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nip47/event_handler.go (1)
88-96: Pruning runs before the request is associated with the app.The goroutine fires right after
last_used_atupdate, butrequestEvent.app_idis set later. That means pruning can keep 1000 old rows, then the current request gets attached, leaving the app above the cap until the next request arrives. For low‑traffic apps, the cap can be violated indefinitely.Please move the prune call to immediately after the successful
app_idupdate for the request event (or after state update), so the just‑created row is included in the limit.🔧 Proposed fix (move prune after app_id update)
- } else { - go svc.pruneRequestEvents(app.ID) - } + } ... err = svc.db. Model(&requestEvent). Update("app_id", app.ID). Error if err != nil { ... return } +go svc.pruneRequestEvents(app.ID)
🧹 Nitpick comments (1)
nip47/prune_test.go (1)
13-48: Strengthen the test to assert the newest events are retained.Right now it only checks the count, so a pruning bug that deletes recent rows would still pass. Consider verifying the oldest/ newest remaining
NostrIdvalues.✅ Example assertion to verify retained rows
nip47Svc.pruneRequestEvents(app.ID) svc.DB.Model(&db.RequestEvent{}).Where("app_id = ?", app.ID).Count(&count) assert.Equal(t, int64(MaxRequestEventsPerApp), count) + +var remaining []db.RequestEvent +svc.DB.Where("app_id = ?", app.ID).Order("id asc").Find(&remaining) +assert.Equal(t, MaxRequestEventsPerApp, len(remaining)) +assert.Equal(t, "event_5", remaining[0].NostrId) +assert.Equal(t, "event_1004", remaining[len(remaining)-1].NostrId)
1 participant
Fixes #21
Implemented automatic pruning to limit stored request events to 1000 per app.
Old events are automatically deleted in the background when new requests come in, preventing disk space issues from spammy apps.
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.